Fix Error constructors to return rather than throw #749

RageKnify · 2020-10-01T11:34:49Z

On master let e = new Error('test'); doesn't work and results in an uncaught error, this PR fixes that by making it so the Error constructors aren't seen as throwing their return value by boa.

Originally opened because SyntaxError.toString and Error.toString were using Value::display while all other error types use Value::to_string.
I don't think this has any effect right now, but I think the errors should all conform to the same pattern.

Has an example you can see TypeError's implementation below:

boa/boa/src/builtins/error/type.rs

Lines 58 to 63 in e83a03a

    
           pub(crate) fn to_string(this: &Value, _: &[Value], ctx: &mut Context) -> Result<Value> { 
        
               let name = this.get_field("name").to_string(ctx)?; 
        
               let message = this.get_field("message").to_string(ctx)?; 
        
               Ok(Value::from(format!("{}: {}", name, message))) 
        
           }

It changes the following:

Change error constructors to wrap their return in Ok() rather than Err()
~~Change conversion of name and message to use Value::to_string rather than Value::display in SyntaxError.toString~~
With the new Builin patterns Error.prototype.toString is only defined once, for the generic Error and all its "subclasses" use that implementation, I've made that version spec compliant.

SyntaxError.toString was using Value::display while all other error types use Value::to_string

codecov · 2020-10-01T11:41:46Z

Codecov Report

Merging #749 into master will increase coverage by 0.07%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   59.13%   59.21%   +0.07%     
==========================================
  Files         155      155              
  Lines        9838     9857      +19     
==========================================
+ Hits         5818     5837      +19     
  Misses       4020     4020

Impacted Files	Coverage Δ
boa/src/context.rs	`70.13% <ø> (ø)`
boa/src/builtins/error/mod.rs	`66.66% <75.00%> (+23.18%)`	⬆️
boa/src/builtins/error/range.rs	`65.00% <100.00%> (ø)`
boa/src/builtins/error/reference.rs	`65.00% <100.00%> (ø)`
boa/src/builtins/error/syntax.rs	`65.00% <100.00%> (ø)`
boa/src/builtins/error/type.rs	`65.00% <100.00%> (ø)`
boa/src/value/operations.rs	`44.67% <0.00%> (+0.07%)`	⬆️
boa/src/builtins/json/mod.rs	`82.43% <0.00%> (+2.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e60d081...d4b9824. Read the comment docs.

Introduces some changes accross files to accommodate for the change Fix: Correct Error.prototype.toString to use Value::to_string rather than Value::Display Test: Add a test case for Error Objects in Object.prototype.toString

boa/src/builtins/error/mod.rs

boa/src/builtins/error/syntax.rs

@HalidOdat

Noted by @HalidOdat in boa-dev#749

HalidOdat · 2020-10-03T23:00:06Z

This should now be unblocked since #722 has been merged :)

RageKnify · 2020-10-04T18:17:56Z

@HalidOdat on Discord we discussed using ? when invoking construct_type_error since it will now return result, but in situations where I know it won't Error, becuase I'm giving &str for the message, should I use expect instead? Using ? might generate unnecessary code.

There are some places where I'm forced to use expect because I'm inside closures which don't return Result.

HalidOdat · 2020-10-04T18:22:05Z

@HalidOdat on Discord we discussed using ? when invoking construct_type_error since it will now return result, but in situations where I know it won't Error, becuase I'm giving &str for the message, should I use expect instead? Using ? might generate unnecessary code.

There are some places where I'm forced to use expect because I'm inside closures which don't return Result.

If we know that it construct_type_error will not give use a Err, then we can return Value and call .expect in the construct_type_error function.

RageKnify · 2020-10-04T18:26:33Z

Ok, that's my opinion too, there's no need for ? if we know it's safe to use expect, I'll make sure all my changes use expect where possible.

Razican · 2020-10-04T21:34:39Z

Ok, that's my opinion too, there's no need for ? if we know it's safe to use expect, I'll make sure all my changes use expect where possible.

The idea here is that if at any point in our code, we should never ever reach a branch, it's good to panic. Wether that branch is an Err(), a None or their opposites (or other things, of course).

A panic should mean "we messed up, this is a bug in Boa 100% for sure", and no JavaScript code should ever cause a panic (we still have to solve this part).

But in this case, I think expect() is the way to go in many places.

boa/src/context.rs

Discussed in boa-dev#749

boa/src/builtins/error/mod.rs

Co-authored-by: Halid Odat <halidodat@gmail.com>

Fix: Make SyntaxError.toString use to_string

3458b3d

SyntaxError.toString was using Value::display while all other error types use Value::to_string

RageKnify mentioned this pull request Oct 1, 2020

Fix output of Object.toString() #737

Merged

RageKnify added 2 commits October 1, 2020 18:42

Merge branch 'master' into fix/syntax_to_string

ba2048f

Refactor: Error constructors to return instead of "throwing"

be659d9

Introduces some changes accross files to accommodate for the change Fix: Correct Error.prototype.toString to use Value::to_string rather than Value::Display Test: Add a test case for Error Objects in Object.prototype.toString

RageKnify changed the title ~~Make SyntaxError.toString use Value::to_string~~ Fix Error constructors to return rather than throw Oct 1, 2020

RageKnify added the builtins PRs and Issues related to builtins/intrinsics label Oct 1, 2020

RageKnify requested review from HalidOdat, Razican, jasonwilliams and Lan2u October 1, 2020 18:36

HalidOdat reviewed Oct 1, 2020

View reviewed changes

boa/src/builtins/error/mod.rs Outdated Show resolved Hide resolved

boa/src/builtins/error/syntax.rs Outdated Show resolved Hide resolved

HalidOdat added the bug Something isn't working label Oct 1, 2020

RageKnify added 2 commits October 2, 2020 00:19

Fix: Make Error.prototype.toString spec compliant

02cd2f8

Noted by @HalidOdat in boa-dev#749

Test: Add a test for Error.toString

21526fb

HalidOdat mentioned this pull request Oct 4, 2020

[Error builtin] does not output "name" property correctly #791

Closed

RageKnify linked an issue Oct 4, 2020 that may be closed by this pull request

[Error builtin] does not output "name" property correctly #791

Closed

RageKnify added 3 commits October 4, 2020 19:33

Merge branch 'master' into fix/syntax_to_string

91310d6

Refactor: Use expect rather than ? where valid

cc9f366

Test: Add 2 test cases for Error.prototype.toString

10dc0f1

HalidOdat reviewed Oct 4, 2020

View reviewed changes

boa/src/context.rs Outdated Show resolved Hide resolved

HalidOdat added this to the v0.11.0 milestone Oct 4, 2020

HalidOdat added the execution Issues or PRs related to code execution label Oct 4, 2020

Refactor: Change construct_$TYPE_error's return back to Value

8975e55

Discussed in boa-dev#749

RageKnify requested a review from HalidOdat October 4, 2020 23:54

HalidOdat reviewed Oct 5, 2020

View reviewed changes

boa/src/builtins/error/mod.rs Outdated Show resolved Hide resolved

Clean up as suggested by @HalidOdat

d4b9824

Co-authored-by: Halid Odat <halidodat@gmail.com>

HalidOdat approved these changes Oct 5, 2020

View reviewed changes

Lan2u approved these changes Oct 5, 2020

View reviewed changes

RageKnify merged commit dc82aa2 into boa-dev:master Oct 5, 2020

RageKnify deleted the fix/syntax_to_string branch October 5, 2020 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Error constructors to return rather than throw #749

Fix Error constructors to return rather than throw #749

RageKnify commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

HalidOdat commented Oct 3, 2020

RageKnify commented Oct 4, 2020

HalidOdat commented Oct 4, 2020

RageKnify commented Oct 4, 2020

Razican commented Oct 4, 2020

	pub(crate) fn to_string(this: &Value, _: &[Value], ctx: &mut Context) -> Result<Value> {
	let name = this.get_field("name").to_string(ctx)?;
	let message = this.get_field("message").to_string(ctx)?;

	Ok(Value::from(format!("{}: {}", name, message)))
	}

Fix Error constructors to return rather than throw #749

Fix Error constructors to return rather than throw #749

Conversation

RageKnify commented Oct 1, 2020 • edited Loading

codecov bot commented Oct 1, 2020 • edited Loading

Codecov Report

HalidOdat commented Oct 3, 2020

RageKnify commented Oct 4, 2020

HalidOdat commented Oct 4, 2020

RageKnify commented Oct 4, 2020

Razican commented Oct 4, 2020

RageKnify commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading